Inside this method we have the information we need to derive imageData and liveImageData, so we shouldn't need to pass them as arguments.

The reason I did this is because since 2e4e00b from @zaneb the imageData is potentially different depending on where it's called - in ValidateManagementAccess it could use the Status image on adopt, but when called via setUpForProvisioning->getUpdateOptsForNode we'll only ever want to use the Spec?

I wasn't sure if unconditionally doing that check against the status was safe, so I tried to replicate the existing logic, I was worried about introducing a new corner case in addition to the one @zaneb was fixing..

This logic is very similar to the BMH NeedsProvisioning() - I wonder if we could use the inverse of that here instead of duplicating nearly the same conditionals?

This used to be the NeedsDeprovisioning() method; we moved it here because having methods on the BMH object is actually bad. They're not inverses of each other.

We may need to make the logic in this function a bit more complicated to deal with switching a host from live image to provisioned image, because in that case we may have a non-nil pointer to a struct with an empty string for the live image and we would want to treat that as though the image was set. We can probably deal with that case in another PR, after we have the basic case working.

This used to be the NeedsDeprovisioning() method; we moved it here because having methods on the BMH object is actually bad. They're not inverses of each other.

This doesn't handle the case where Spec.LiveImage is set and Status.Provisioning.Image.URL is also set, in which case we want to return true.

Similarly, below we don't handle the case where Spec.Image is set and Status.Provisioning.LiveImage.URL is also set.

I think the correct logic is something like:

switch {
case hsm.Host.Spec.LiveImage != nil:
    if hsm.Host.Spec.LiveImage.URL == "" {
        return true
    }
    if hsm.Host.Status.Provisioning.Image.URL != "" {
        return true
    }
    if hsm.Hosts.Status.Provisioning.LiveImage.URL == "" {
        return false
    }
    if hsm.Host.Spec.LiveImage != nil && hsm.Host.Spec.LiveImage.URL != hsm.Host.Status.Provisioning.LiveImage.URL {
        return true
    }
}
case hsm.Host.Spec.Image != nil:
    if hsm.Host.Spec.Image.URL == "" {
        return true
    }
    if hsm.Host.Status.Provisioning.LiveImage.URL != "" {
        return true
    }
    if hsm.Host.Status.Provisioning.Image.URL == "" {
        return false
    }
    if hsm.Host.Spec.Image.URL != hsm.Host.Status.Provisioning.Image.URL {
        return true
    }
default:
    return true
}
return false

Assuming that hsm.Host.Spec.LiveImage and hsm.Host.Spec.Image cannot both be non-nil. If they both can be non-nil it quickly devolves even further into a nightmare. Seeing all this duplicated/permuted logic makes me wonder whether we should just have an ISO flag in the regular Image struct instead. At least in the Status maybe?

Thanks for the review @zaneb - In the docstring I do mention that only Image or LiveImage should be set, but I agree long term they should be validated as mutually exclusive.

Re the ISO flag, that was my original proposal but this approach was preferred since it's possible we might support a live kernel/ramdisk in future, and the checksum argument to the current Image section currently isn't applicable to the LiveImage case: metal3-io/metal3-docs#150 (comment)

Don't we also need to remove any options corresponding to the other type of image?

Yeah, I think there are several corner cases in the Image->LiveImage and LiveImage->Image case which I've not yet tested or accounted for in the code.

I can attempt to do that in this PR, or as previously mentioned by @dhellmann we might want to land this first iteration then take care of that as a followup, so it's easier to review?